Skip to content

Conversation

jkruse14
Copy link

No description provided.

@jkruse14 jkruse14 self-assigned this Oct 30, 2019
Copy link

@lagnat lagnat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break dev and master? package.json doesn't specify a branch or hash, if that's even possible.

index.js Outdated
// TODO: send PR to serverless-offline to export this
const functionHelper = require('serverless-offline/src/functionHelper')
const createLambdaContext = require('serverless-offline/src/createLambdaContext')
const createLambdaContext = require('serverless-offline/src/LambdaContext')
Copy link

@karlsnyder0 karlsnyder0 Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like-

let createLambdaContext = null;
try {
// Support serverless-offline prior to version x.y.z.
createLambdaContext = require('serverless-offline/src/createLambdaContext')
} catch(e) {
try {
// Support serverless-offline after version x.y.z.
createLambdaContext = require('serverless-offline/src/LambdaContext')
} catch(e) {
// burgers and fries
}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed exactly that. Long story, long.. Houston doesn't need ssl for serverless iot local anymore so we may not need the fork of serverless-iot-local which then means we can send this fix to tradle as a PR instead of maintaining the fork.. and no, he didn't fix it yet either. Another ticket for another time. We'll defer the upgrade of serverless-offline.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still use iot for local serverless-offline testing. We likely still need this plugin.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something in the fork that we need for off-line testing?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need the plugin for testing.
image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-11-01 at 7 47 03 AM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we fork anything? Keep this plugin. Problem solved.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-11-01 at 8 09 17 AM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're going to need to explain better than word circling (or squaring).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going with the answer you gave me yesterday.

Copy link

@lagnat lagnat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send this new PR upstream too?

@jkruse14 jkruse14 merged commit 74aaa2f into ssl Dec 6, 2019
@jkruse14 jkruse14 deleted the ssl-2 branch December 6, 2019 15:02
@jkruse14 jkruse14 restored the ssl-2 branch December 6, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants